-
Notifications
You must be signed in to change notification settings - Fork 8k
[PFA 2/n] Support PFA functionality #20848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
TimWolla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at 43/143 files with a mix of tests and C code.
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| Fatal error: Uncaught ArgumentCountError: f(): Argument #2 ($b) not passed in %s:6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to improve the error message here to make it clear that this is happening during partial application?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not specific to partials: https://3v4l.org/biQNc
|
|
||
| ?> | ||
| --EXPECT-- | ||
| ArgumentCountError: Closure::__invoke(): Argument #3 ($c) must be passed explicitly, because the default value is not known |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a user the error message reads odd, because stating that the default value is “unknown” implies that it is unclear whether there is one. If this is a technical restriction this should be rephrased to something like “cannot be determined for Closures” or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is not specific to partials or Closures: https://3v4l.org/Kl9M1
It is thrown when a parameter with position n < func_num_args() is not specified, and has a default value of UNKNOWN, like here:
php-src/ext/standard/basic_functions.stub.php
Line 1689 in 643cf62
| function array_keys(array $array, mixed $filter_value = UNKNOWN, bool $strict = false): array {} |
The reason it can happen is that functions assume that all arguments bellow func_num_args() are specified, but due to named args it's not always the case. So the engine fills the unspecified args bellow func_num_args() with their default value, but that's not possible with UNKNOWN.
Closure::__invoke() is a special case of this, as the default value is always unknown for this function (this affects only calling __invoke() explicitly).
I'm realizing that in the context of PFAs, we could probably avoid this by making placeholders without a default value required. I can look at this in a separate PR after this one.
TimWolla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
74/144
|
|
||
| Deprecated: Using "_" as a type name is deprecated since 8.4 in %s on line 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this error message being emitted here is not great, because this is not actionable by the user who have written the PFA call (it can only be fixed by the author of the function being called).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
Possible alternative ways to handle these errors:
- Prefix errors with "During compilation of partial application:"
- Discard non-fatal errors emitted during compilation of PFAs
- Implement workarounds to avoid emitting any error
The second one seems to be the most sensible, as the errors are actually related to the function being called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't imagine what else than Foo would be returned for get_called_class(). Should this test involve inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is just testing basic functionality. That get_called_class() works as expected is obvious with the current implementation, but may not have been with the previous one where the call was made in a more custom way.
RFC: https://wiki.php.net/rfc/partial_function_application_v2 Co-authored-by: Joe Watkins <[email protected]>
0b7a4a5 to
2cf2ae1
Compare
RFC: https://wiki.php.net/rfc/partial_function_application_v2
This follows #20717. This implements most of the RFC, except PFAs in constant expressions, and some optimization.
I wanted to split this PR more, but I didn't find a way to achieve this that makes sense.
A partial application is compiled to the usual sequence of function call
opcodes (
INIT_FCALL,SEND_VAR, etc), but the sequence ends with aCALLABLE_CONVERT_PARTIALopcode instead ofDO_FCALL, similarly tofirst class callables. Placeholders are compiled to
SEND_PLACEHOLDERopcodes:SEND_PLACEHOLDERsets the argument slot type to_IS_PLACEHOLDER.CALLABLE_CONVERT_PARTIALuses the information available on the stack tocreate a Closure and return it, consuming the stack frame in the process
like an internal function call.
We create the Closure by generating the relevant AST and compiling it to an
op_array.
The op_array is cached in the Opcache SHM and inline caches. The SHM key is prefixed with
pfa://to avoid any collision with actual files. When Opcache is disabled, we cache PFA op_arrays a global hash table. This is mainly useful for polymorphic PFAs, as the inline cache stores only a single entry.